Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

scout hint: tell user logging in is required #2262

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

laurazard
Copy link
Contributor

The docker scout quickview hint, displayed after a user pulls or builds an image, only works if the user is logged in to Hub.

What I did

Check if user is logged in, and make hint more explicit/call this requirement out if not.

Logged in (same behaviour as before):

image

Logged out (add message about logging in):

image

(not mandatory) A picture of a cute animal, if possible in relation with what you did

image

@laurazard
Copy link
Contributor Author

/cc @sam-thibault @thaJeztah @dvdksn @neersighted @rumpl @vvoland (pinging y'all re: wording ideas)

@thaJeztah
Copy link
Member

So, looking at this hint:

  • logging won't be needed until I run that command
  • and it's unlikely that a user will now do a docker login if they're not planning to run the related docker scout command

I'm slightly wondering if this is "too much information" at this moment in time, and cluttering the hint (hiding away the relevant information in the noise). Effectively, this is listing prerequisites to run the docker scout command, and we should not end up with "make sure you have a working internet connection: to connect to the internet ..."

So, silly idea:

  • Can we make docker scout check if the user is authenticated before the command is executed?
  • Or.. make it "try", and fallback to asking the user to authenticate?
  • ^^^ we should actually consider doing the same for docker pull (etc)

That would also allow us to include some of the wording we added in docker/cli#4478

Thinking of a flow like:

$ docker scout quickview hello-world
Log in with your Docker ID or email address to use the scout service. If you
don't have a Docker ID, head over to https://hub.docker.com to create one.
You can log in with your password or a Personal Access  Token (PAT). Using
a limited-scope PAT grants better security and is required for organizations
using SSO. Learn more at https://docs.docker.com/go/access-tokens/

Username:
Password:

(we should consider changing Password to Passphrase or Passphrase or PAT, but probably should do so across the board)

@laurazard
Copy link
Contributor Author

@eunomie thoughts on @thaJeztah's idea?

@eunomie
Copy link
Contributor

eunomie commented Aug 22, 2023

I can see why we want to add more context about the docker scout commands.
To add (login required) might be OK, let me think a bit more about it. But we probably don't need the second line.

Right now, that's what you have if you run a scout command without to be logged in:

$ docker scout qv alpine:latest
ERROR   Status: login using Docker Desktop or 'docker login' command: no login, Code: 1

That's not a really good user experience. However I'm not sure about the addition of the login inside docker scout.
I think the way it's working everywhere right now, is it's up to the user to login (CLI or desktop for instance).
So I don't want to include that logic here.

But to improve the login message 💯

@eunomie
Copy link
Contributor

eunomie commented Aug 22, 2023

I can change the docker scout login error that way:

image

But I wonder if it isn't too much details for this command, as this message it's more for docker login.

cli/mobycli/scout_suggest.go Outdated Show resolved Hide resolved
cli/mobycli/scout_suggest.go Outdated Show resolved Hide resolved
The `docker scout quickview` hint, displayed
after a user pulls or builds an image, only
works if the user is logged in to Hub.

Check if user isn't logged in, and make hint
more explicit in this case.

Signed-off-by: Laura Brehm <[email protected]>
@laurazard laurazard marked this pull request as ready for review August 25, 2023 08:13
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@milas milas merged commit 8a400d0 into docker-archive:main Aug 25, 2023
13 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cli cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants